*: support creating a user without password #9817

mitake · 2018-06-07T07:52:07Z

This commit adds a feature for creating a user without password. The
purpose of the feature is reducing attack surface by configuring bad
passwords (CN based auth will be allowed for the user).

The feature can be used with --no-password of etcdctl user add
command.

Fix #9590

mitake · 2018-06-07T07:52:49Z

This PR isn't tested yet. I'll do it and add e2e test cases later.

/cc @crielly @joelegasse

mitake · 2018-06-13T05:21:22Z

@joelegasse I added the test cases, could you take a look? It seems that the travis failures aren't related to the change.

jpbetz · 2018-06-13T19:57:40Z

Let's document this carefully. The behavior of the system for users with no password needs to be clear so that the security implications can be reasoned about easily.

mitake · 2018-06-14T06:34:47Z

@jpbetz thanks for pointing out, I'll update the doc later.

mitake · 2018-06-14T10:19:35Z

Updated the doc: https://github.com/coreos/etcd/pull/9817/files#diff-fa0a9438986edbb6abed85fbeea1753c
Could you take a look? @jpbetz

jpbetz · 2018-06-14T16:28:45Z

Docs look great @mitake. Thanks!

gyuho · 2018-06-14T16:32:49Z

@mitake Can you fix the CI failure https://travis-ci.org/coreos/etcd/jobs/392115574#L898-L912?

jpbetz

Code and doc changes look good to me. Just want to check on the backward compatibility before approving.

Thanks!

jpbetz · 2018-06-14T16:33:13Z

clientv3/auth.go

@@ -61,7 +61,7 @@ type Auth interface {
 	AuthDisable(ctx context.Context) (*AuthDisableResponse, error)

 	// UserAdd adds a new user to an etcd cluster.
-	UserAdd(ctx context.Context, name string, password string) (*AuthUserAddResponse, error)
+	UserAdd(ctx context.Context, name string, password string, noPassword bool) (*AuthUserAddResponse, error)


This is a source backward incompatible change in the client. Is this acceptable? The only alternative I can think of would be to introduce an additional UserAddNoPassword function, or similar.

cc @gyuho for compatibility expectations of changes to client

@jpbetz Oh good catch.

UserAddNoPassword sounds better for compatibilities.

/cc @mitake

One other alternative would be to introduce a UserAddWithOptons(..., opts... UserAddOption) function as well as a WithNoPassword UserAddOption. This would future proof the API against additions of more UserAdd parameters in the future. I'm not sure if that's justified here or not.

Thanks for pointing out. I'll introduce UserAddWithOptons. It would be good for expanding in the future as @jpbetz says.

jpbetz · 2018-06-14T16:33:42Z

clientv3/auth.go

-func (auth *authClient) UserAdd(ctx context.Context, name string, password string) (*AuthUserAddResponse, error) {
-	resp, err := auth.remote.UserAdd(ctx, &pb.AuthUserAddRequest{Name: name, Password: password}, auth.callOpts...)
+func (auth *authClient) UserAdd(ctx context.Context, name string, password string, noPassword bool) (*AuthUserAddResponse, error) {
+	resp, err := auth.remote.UserAdd(ctx, &pb.AuthUserAddRequest{Name: name, Password: password, NoPassword: noPassword}, auth.callOpts...)


same as above. Should we retain backward compatibility?

mitake · 2018-06-18T08:23:17Z

@jpbetz @gyuho updated for not breaking source level compatibility. Could you take a look?

joelegasse

Personally, this seems more complex where a simple length check on the password should work just as well.

I think having the extra flag in addition to always sending in a password argument is strange. At the CLI level, I would still suggest having the --no-password flag just to signal that it's intended, but once at the protobuf/API level, an empty password could signal "disable password auth for this user".

Then, instead of checking an options field in the user for NoPassword, it's just if len(u.Password) == 0 {}.

joelegasse · 2018-06-18T14:34:48Z

auth/store.go

-			)
-		} else {
-			plog.Errorf("failed to hash password: %s", err)
+	hashed := make([]byte, 0)


Minor note: var hashed []byte should be used here instead of make-ing a zero-length slice

OK, I'll fix it soon

mitake · 2018-06-19T04:53:55Z

@joelegasse handling an empty password as a special case breaks existing semantics of the RPC so I don't want to do it. How do you think? @jpbetz @gyuho

mitake · 2018-06-19T06:24:46Z

I couldn't reproduce the travis fail on my local environment. It would be freaky.

joelegasse · 2018-07-23T14:49:46Z

@mitake Do you have time to rebase this PR? You were correct that the current RPC allows users to be created/authenticated with an empty password, so that wouldn't be a backwards-compatible change.

If not, I can take this over and get it cleaned up for more thorough testing.

Suggested changes were not feasible

mitake · 2018-07-23T15:09:02Z

@joelegasse probably I will be able to rebase this weekend. Do you need to test it earlier?

joelegasse · 2018-07-23T15:21:21Z

No, that should be fine. Thank you :-)

mitake · 2019-01-09T15:43:04Z

@gyuho I'm still seeing a confusing build error like below, do you know why does this happen? It would be related to recent path change? I'll work on it probably tomorrow.

$ make
github.com/coreos/etcd/clientv3
# github.com/coreos/etcd/clientv3
../../github.com/coreos/etcd/clientv3/auth.go:121:72: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.AuthEnable
../../github.com/coreos/etcd/clientv3/auth.go:126:74: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.AuthDisable
../../github.com/coreos/etcd/clientv3/auth.go:131:152: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserAdd
../../github.com/coreos/etcd/clientv3/auth.go:136:144: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserAdd
../../github.com/coreos/etcd/clientv3/auth.go:141:86: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserDelete
../../github.com/coreos/etcd/clientv3/auth.go:146:122: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserChangePassword
../../github.com/coreos/etcd/clientv3/auth.go:151:104: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserGrantRole
../../github.com/coreos/etcd/clientv3/auth.go:156:80: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserGet
../../github.com/coreos/etcd/clientv3/auth.go:161:72: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserList
../../github.com/coreos/etcd/clientv3/auth.go:166:106: cannot use auth.callOpts (type []"github.com/coreos/etcd/vendor/google.golang.org/grpc".CallOption) as type []"go.etcd.io/etcd/vendor/google.golang.org/grpc".CallOption in argument to auth.remote.UserRevokeRole
../../github.com/coreos/etcd/clientv3/auth.go:166:106: too many errors
Makefile:15: recipe for target 'build' failed
make: *** [build] Error 2

mitake · 2019-01-09T15:48:46Z

@wenjiaswe This PR add a new member to etcdserverpb.AuthenticateRequest. I'm not sure but would like to let you know about this because it might affect your downgrade functionality. The functionality might need to care about WAL entries which have the new member (IIRC, protobuf generated code can ignore a member which isn't in a definition?).

mitake · 2019-04-30T15:37:02Z

@gyuho @jpbetz sorry for my late update. I fixed the build problem so the PR is ready to be reviewed again. I think the branch includes fixes for all comments from you. Could you take a look?

mitake · 2019-05-01T10:22:48Z

The unit test is failing because of data race. I opened a new issue here: #10697 The issue seems to be independent from this PR.

codecov-io · 2019-05-01T10:42:32Z

Codecov Report

Merging #9817 into master will increase coverage by 0.88%.
The diff coverage is 26.92%.

@@            Coverage Diff             @@
##           master    #9817      +/-   ##
==========================================
+ Coverage    62.3%   63.19%   +0.88%     
==========================================
  Files         392      392              
  Lines       37158    37183      +25     
==========================================
+ Hits        23152    23498     +346     
+ Misses      12438    12102     -336     
- Partials     1568     1583      +15

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/user_command.go	`34.81% <4.54%> (-0.96%)`	⬇️
auth/store.go	`68.5% <42.3%> (+17.71%)`	⬆️
clientv3/auth.go	`62.33% <50%> (-2.53%)`	⬇️
pkg/transport/timeout_conn.go	`60% <0%> (-20%)`	⬇️
pkg/adt/interval_tree.go	`78.67% <0%> (-9.91%)`	⬇️
etcdserver/api/v3rpc/lease.go	`69.31% <0%> (-5.69%)`	⬇️
proxy/grpcproxy/watcher.go	`85.71% <0%> (-4.09%)`	⬇️
clientv3/concurrency/election.go	`79.68% <0%> (-2.35%)`	⬇️
etcdserver/api/v3rpc/watch.go	`76.14% <0%> (-2.29%)`	⬇️
etcdserver/api/rafthttp/msgappv2_codec.go	`69.56% <0%> (-1.74%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc6885d...96a7ff0. Read the comment docs.

mitake · 2019-05-17T17:02:40Z

This PR depends on the fix introduced in #10739

mitake · 2019-05-29T15:54:01Z

@gyuho @jpbetz now all CIs are passed. Could you take a look?

gyuho · 2019-05-29T16:49:42Z

@mitake Sorry for delay. Will take a look by today.

gyuho · 2019-05-30T01:23:02Z

@mitake Can you also update changelog, with rebasing to trigger CIs?

This commit adds a feature for creating a user without password. The purpose of the feature is reducing attack surface by configuring bad passwords (CN based auth will be allowed for the user). The feature can be used with `--no-password` of `etcdctl user add` command. Fix etcd-io#9590

mitake · 2019-05-30T13:45:36Z

@gyuho done, could you take a look? I found a fleaky test which wouldn't be related to this PR. Issue for the test is here #10774

jingyih · 2019-06-06T00:07:22Z

Hi @mitake, I noticed an e2e test failure which might be related to this PR: https://semaphoreci.com/etcd-io/etcd/branches/pull-request-10797/builds/3

I took a quick look, maybe this function call also needs to be updated?
https://github.com/mitake/etcd/blob/96a7ff0a62ce3f52aad258289bd0f25d1a781a1b/tests/e2e/v3_curl_test.go#L199

mitake · 2019-06-06T14:18:36Z

@jingyih thanks a lot for pointing out! Yeah probably that's the reason. It is a little bit surprising for me because such an uninitialized variable is zero cleared by go. So the behavior of the function would be deterministic and the tests rely on it shouldn't be flaky. Probably protobuf layer has a pitfall related to this? I'll take a look but please let me know if you know something related to the behavior.

Anyway I opened #10800 Could you take a look?

BTW the link to the result of semaphore CI seems to be expired... I hope semaphore to keep results longer :(

jingyih · 2019-06-06T18:16:12Z

We should probably move all CI tests to Travis, now that it can run 13 jobs in parallel.

The failed tests was TestV3CurlAuth and TestV3CurlAuthClientTLSCertAuth. They are fixed by #10800.

gyuho requested a review from joelegasse June 7, 2018 17:55

mitake force-pushed the no-password branch 4 times, most recently from e52ceb2 to bc20594 Compare June 13, 2018 04:51

mitake changed the title ~~WIP, DO NOT MERGE *: support creating a user without password~~ *: support creating a user without password Jun 13, 2018

mitake force-pushed the no-password branch from bc20594 to e779231 Compare June 14, 2018 07:08

jpbetz suggested changes Jun 14, 2018

View reviewed changes

mitake force-pushed the no-password branch from e779231 to c8165db Compare June 18, 2018 08:22

mitake force-pushed the no-password branch 2 times, most recently from 357a5b7 to 29a8e61 Compare June 18, 2018 09:36

joelegasse previously requested changes Jun 18, 2018

View reviewed changes

mitake force-pushed the no-password branch 2 times, most recently from 3709a11 to f9b7766 Compare June 19, 2018 05:33

mitake force-pushed the no-password branch from f9b7766 to a4e87dc Compare June 20, 2018 09:28

mitake force-pushed the no-password branch from a4e87dc to 0e6736c Compare July 29, 2018 05:38

mitake force-pushed the no-password branch 2 times, most recently from b3dcbeb to fe92b77 Compare January 9, 2019 15:41

mitake force-pushed the no-password branch from fe92b77 to 0cdfb97 Compare April 30, 2019 15:34

mitake force-pushed the no-password branch 2 times, most recently from d8ea60d to bb20f87 Compare May 1, 2019 09:32

mitake mentioned this pull request May 1, 2019

Potential data race in TestResumeElection #10697

Closed

mitake force-pushed the no-password branch from bb20f87 to 9c46fb4 Compare May 29, 2019 15:19

mitake and others added 4 commits May 30, 2019 21:59

e2e: add test cases for a user without password

8257dfd

auth: add a unit test for creating a user with no password

54b09d4

CHANGELOG: describe about user with no password

96a7ff0

mitake force-pushed the no-password branch from 9c46fb4 to 96a7ff0 Compare May 30, 2019 13:03

mitake mentioned this pull request May 30, 2019

nil pointer dereference in TestLeasingDisconnectedGet #10774

Closed

gyuho approved these changes Jun 1, 2019

View reviewed changes

gyuho merged commit cdca488 into etcd-io:master Jun 1, 2019

mitake deleted the no-password branch June 1, 2019 06:06

ahrtr mentioned this pull request Mar 9, 2023

server/auth: fix auth panic bug when user changes password #15432

Merged

*: support creating a user without password #9817

*: support creating a user without password #9817

Conversation

mitake commented Jun 7, 2018 • edited Loading

mitake commented Jun 7, 2018

mitake commented Jun 13, 2018

jpbetz commented Jun 13, 2018

mitake commented Jun 14, 2018

mitake commented Jun 14, 2018

jpbetz commented Jun 14, 2018

gyuho commented Jun 14, 2018

jpbetz left a comment

Choose a reason for hiding this comment

jpbetz Jun 14, 2018

Choose a reason for hiding this comment

gyuho Jun 14, 2018

Choose a reason for hiding this comment

jpbetz Jun 14, 2018

Choose a reason for hiding this comment

mitake Jun 15, 2018

Choose a reason for hiding this comment

jpbetz Jun 14, 2018

Choose a reason for hiding this comment

mitake commented Jun 18, 2018

joelegasse left a comment

Choose a reason for hiding this comment

joelegasse Jun 18, 2018

Choose a reason for hiding this comment

mitake Jun 19, 2018

Choose a reason for hiding this comment

mitake commented Jun 19, 2018

mitake commented Jun 19, 2018

joelegasse commented Jul 23, 2018

mitake commented Jul 23, 2018 • edited Loading

joelegasse commented Jul 23, 2018

mitake commented Jan 9, 2019 • edited Loading

mitake commented Jan 9, 2019

mitake commented Apr 30, 2019

mitake commented May 1, 2019

codecov-io commented May 1, 2019 • edited Loading

Codecov Report

mitake commented May 17, 2019

mitake commented May 29, 2019

gyuho commented May 29, 2019

gyuho commented May 30, 2019

mitake commented May 30, 2019

jingyih commented Jun 6, 2019

mitake commented Jun 6, 2019

jingyih commented Jun 6, 2019 • edited Loading

mitake commented Jun 7, 2018 •

edited

Loading

mitake commented Jul 23, 2018 •

edited

Loading

mitake commented Jan 9, 2019 •

edited

Loading

codecov-io commented May 1, 2019 •

edited

Loading

jingyih commented Jun 6, 2019 •

edited

Loading